-
Notifications
You must be signed in to change notification settings - Fork 411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Make the anonymous field of the groupName marker optional #1000
✨ Make the anonymous field of the groupName marker optional #1000
Conversation
Welcome @twz123! |
Hi @twz123. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/ok-to-test |
@twz123 Implementation looks good. Let's please add some test coverage. (I think somewhere below pkg/crd/testdata something that uses groupName=) |
Upstream introduced the use of empty groupName markers in k/k PR 125162. This breaks groupName parsing because its field is non-optional. Add an additional mustOptional function that takes anonymous field definitions and turns them into optional ones. Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
31c93d2
to
7c1db6b
Compare
Done! |
/retest |
@@ -22,7 +22,7 @@ import ( | |||
|
|||
func init() { | |||
AllDefinitions = append(AllDefinitions, | |||
must(markers.MakeDefinition("groupName", markers.DescribesPackage, "")). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sttts Does it sound reasonable for you to just adjust the marker here to accept an "empty" groupName marker ("// +groupName=") without any additional handling for the empty string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context: Without this PR controller-gen fails like this now (with API types that use types from k8s.io/api@v0.31.0-alpha.3)
/home/prow/go/pkg/mod/k8s.io/api@v0.31.0-alpha.3/core/v1/doc.go:21:1: missing argument "" (at )
/home/prow/go/pkg/mod/k8s.io/api@v0.31.0-alpha.3/core/v1/doc.go:21:1: missing argument "" (at )
https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/10803/pull-cluster-api-verify-main/1811372226943913984
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without any additional handling for the empty string?
This is about embedding corev1 types in some CRD, right? So the groupName of the corev1 package does not really play a role for the CRD, does it? So yes, I think just parsing it without falling over, and then ignoring it outside of actual CRD API package sounds right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is about embedding corev1 types in some CRD, right?
Yes exactly
So the groupName of the corev1 package does not really play a role for the CRD, does it?
This is my understanding, yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if anyone is doing it, but theoretically folks could use controller-gen to generate CRDs for types of the k/k core API. In that case they would end up with an empty group in the CRD with groupName=
(see the test in this PR: pkg/crd/testdata/testdata._cronjobs.yaml)
Not sure which group would be correct in this (probably hypothetical) case. But folks could just specify whatever group they want via groupName, so I assume it's fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, thx!
I think then let's do the same in controller-gen for now. We can still modify this behavior later (or offer some additional markers) if people want something else for CRD generation for the core API group
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@twz123 WDYT about handling the empty group here:
controller-tools/pkg/crd/spec.go
Line 73 in b370e9b
Name: defaultPlural + "." + groupKind.Group, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current name generated in the test case also seems to be invalid:
The CustomResourceDefinition "cronjobs." is invalid:
* metadata.name: Invalid value: "cronjobs.": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')
* metadata.name: Invalid value: "cronjobs.": must be spec.names.plural+"."+spec.group
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sttts I was trying to deploy a CRD with name & group like this, but I'm getting these errors with a regular kube-apiserver (kindest/node:v1.30.0)
The CustomResourceDefinition "cronjobs.core" is invalid:
* metadata.name: Invalid value: "cronjobs.core": must be spec.names.plural+"."+spec.group
* spec.group: Required value
CRD:
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: (devel)
name: cronjobs.core
spec:
group: ""
names:
kind: CronJob
listKind: CronJobList
plural: cronjobs
singular: cronjob
scope: Namespaced
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Kube does is not allowed today. You cannot mix native and CRD api groups.
/lgtm |
LGTM label has been added. Git tree hash: 98d1e914f29072f51bc71936ebe0a015b8994ee9
|
Let's go ahead with this fix to unblock folks that just want to generate CRD for "non-core" apiGroups. I think this should be 99,9% of controller-gen users. Let's see if we want to do a follow-up based on the discussion here: #1000 (comment) |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer, twz123 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Upstream introduced the use of empty groupName markers in kubernetes/kubernetes#125162. This breaks groupName parsing because its field is non-optional. Add an additional
mustOptional
function that takes anonymous field definitions and turns them into optional ones.